Handle pre-contest clarifications #3174

vmcj · 2025-10-26T18:02:40Z

I looked into if we can let both API & UI get the data via a service (to remove having to handle everything twice) but that would require a lot of rewrites which need a bit of further thought.

The first 2 commits are unrelated, I'll open another PR for those but they broke my local build.

sentry · 2025-10-26T18:02:57Z

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: webapp/src/Controller/API/ClarificationController.php

Function	Unhandled Issue
`App\Controller\API\ClarificationController::addAction`	Symfony\Component\HttpKernel\Exception\BadRequestHttpException: Problem 'A' not found. /src/Controller/API/ClarificationController.php in App\Controller... `Event Count:` 3

meisterT · 2025-10-26T19:05:21Z

I would only do the warning, but include implications for the API in the message.

vmcj · 2025-10-26T19:30:52Z

I would only do the warning, but include implications for the API in the message.

So drop all other commits? I personally think keeping the UI/API uniform is an improvement.

There are good reasons to already send the clarification, so we don't disable the feature but warn the user.

In the Jury UI we can warn with a flash message, for API we can't easily prevent automated tools from unintentionally disclosing the information. We provide both relevant times, either contest start or now to keep this easy to copy/paste. The format is already in the timezone of the contest to make sure validation passes. In theory this can still give problems in case you submit a clarification and afterwards the contest starts later, it's up to the contest admin to change the database in such cases as we don't have a reltime for clarifications.

We already disclose those in the API, so keep this consistent. This does result in the `contest time` of that clarification to be negative but thats factually true.

We allow via the API, this just makes the behaviour uniform.

It looks ugly in some cases and less professional in others where printing happened much earlier. The exact time is not relevant and could lead to discussions with teams.

…start

We do for the jury so they can still see the interface as a time would see it.

webapp/src/Controller/Jury/ClarificationController.php

meisterT · 2025-10-27T19:17:41Z

webapp/src/Controller/Jury/ClarificationController.php

+    private function warnClarificationBeforeContestStart(): void
+    {
+        $cc = $this->dj->getCurrentContest();
+        $message = "Generic clarifications are visible before contest start.";


not just generic, right?

In the later commits I don't disclose clarifications about problems, I know you guys had opinions on this but I think we should try the referential integrity. If we don't like the extra code path for maintainability we can discuss it now and change this message.

I think this PR does many things at once, some of which can be merged easily and maybe should be lifted to their own PRs so that they can be merged already.

I think this PR does many things at once, some of which can be merged easily and maybe should be lifted to their own PRs so that they can be merged already.

Which ones would you approve? I really like to close this PR soon as getting merge conflicts here is risky.

meisterT · 2025-10-27T19:18:09Z

webapp/src/Controller/Jury/ClarificationController.php

        #[MapQueryParameter]
        ?string $teamto = null,
    ): Response {
+        $this->warnClarificationBeforeContestStart();


in addition, I was thinking whether we should add a modal dialog on submission warning of the implications

I don't like this for 2 reasons... modals are more work and in case you know what you're doing this feels a little patronizing. You have more experience with judges so if you think it's smarter I can take a look how much work it is.

This a very edge case with some consequences, so yes, I would prefer to put into the face of the jury. Under normal operation, nobody should see the modal dialog.

This a very edge case with some consequences, so yes, I would prefer to put into the face of the jury. Under normal operation, nobody should see the modal dialog.

Sounds reasonable, I'll spend the time on it.

meisterT · 2025-10-27T19:21:35Z

webapp/templates/team/partials/index_content.html.twig

    <h2 id="contestnotstarted" class="text-center">
        Contest {{ contest | printContestStart }}
    </h2>
+    {% if clarifications is not empty %}


Thinking a little bit more: when do we send out "static events" in the event feed? Do we do that on state changes? Then hiding them in the API is perhaps not too much code complexity, but one would have to implement it to really see.

I don't think I understand.

Probably best described in person - @nickygerritsen, is my understanding correct that static events are created on state changes?

Clarifications are not static events. But we send them out here, so when a dependency is missing basically.

hmmm, needs more thought

meisterT · 2025-10-27T19:22:57Z

webapp/templates/team/partials/clarifications_team.html.twig

I would rather reject clar requests before contest start through the API

So instead of letting teams ask them via UI (because they can via API) disallow both via UI & API?

I think I'm fine with that, I want the jury to still be able to send them via API because of imports and suspected we didn't want the extra branch. I did not really like having to add this to the UI but found consistency important.

Yes, there is no point in asking for clarification for a contest where no data is supposed to be known yet to teams.

I want the jury to still be able to send them via API because of imports

Do we have a way to import clarifications through problem upload or what do you mean by "imports" here?

Yes, there is no point in asking for clarification for a contest where no data is supposed to be known yet to teams.

I think can I go to the toilet is always an important one. But I'm fine with either way.. either the extra branch in the UI or the extra branch in the API.

Do we have a way to import clarifications through problem upload or what do you mean by "imports" here?

I'm not sure if we have it yet, but I think it's something we should have. In BAPC prelims (multisite over different days) the jury now updates the PDF. But I can imagine a year where we would just also upload the clars directly in case as they don't have the time.

meisterT · 2025-10-27T19:24:14Z

webapp/src/Twig/TwigExtension.php

        if ($contest !== null && $this->config->get('show_relative_time')) {
            $relativeTime = $contest->getContestTime((float)$datetime);
+            if ($relativeTime < 0 && $squash) {
+                return "Before contest";


wonder if this makes sense in general and not just here?

This should just display it in the table, I didn't find other locations yet (also didn't really search) but in general I agree. Probably best to move this commit out and do this in a separate PR to find the other locations.

meisterT · 2025-10-27T19:25:27Z

I would only do the warning, but include implications for the API in the message.

So drop all other commits? I personally think keeping the UI/API uniform is an improvement.

I misunderstood what was happening, and left now individual comments on the code

vmcj added 7 commits October 27, 2025 13:34

Warn jury for clarifications before contest start

0071234

There are good reasons to already send the clarification, so we don't disable the feature but warn the user.

Display pre-contest clarifications

4706e75

We already disclose those in the API, so keep this consistent. This does result in the `contest time` of that clarification to be negative but thats factually true.

Allow teams to request clarifications before the contest

1f77c9a

We allow via the API, this just makes the behaviour uniform.

Don't display clarification time from before contest

8342cf2

It looks ugly in some cases and less professional in others where printing happened much earlier. The exact time is not relevant and could lead to discussions with teams.

Don't reference undisclosed problems in clarifications before contest…

c8c97ec

…start

Don't display clarifications about non-disclosed problems yet

3a60f88

We do for the jury so they can still see the interface as a time would see it.

vmcj force-pushed the jury_clarification_warning branch from 9b278b2 to 3a60f88 Compare October 27, 2025 12:36

meisterT reviewed Oct 27, 2025

View reviewed changes

Handle pre-contest clarifications #3174

Are you sure you want to change the base?

Handle pre-contest clarifications #3174

Uh oh!

Conversation

vmcj commented Oct 26, 2025

Uh oh!

sentry bot commented Oct 26, 2025

🔍 Existing Issues For Review

Uh oh!

meisterT commented Oct 26, 2025

Uh oh!

vmcj commented Oct 26, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meisterT commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants